Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Mul operator for Quaternion + Vector3. #894

Merged
merged 1 commit into from
Sep 14, 2024

Conversation

theKidOfArcrania
Copy link
Contributor

Fixes #893.

@Bromeon Bromeon added feature Adds functionality to the library c: core Core components labels Sep 13, 2024
@Bromeon
Copy link
Member

Bromeon commented Sep 13, 2024

Thank you! The CI errors seem unrelated, I'll fix them tomorrow.

Could you maybe add a simple #[itest] in quaternion_test.rs? 1-2 multiplications and hardcoded values are OK, see existing code.

Please also squash to 1 commit in the end 🙂

@GodotRust
Copy link

API docs are being generated and will be shortly available at: https://godot-rust.github.io/docs/gdext/pr-894

@theKidOfArcrania
Copy link
Contributor Author

Yep can do, though quick question, is there a reason why itests are being used instead of rust's builtin unit test?

Copy link
Member

@Bromeon Bromeon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the reason is that we need Godot to run, and Rust's #[test] isn't configurable to change the "host" application.
See also https://godot-rust.github.io/book/contribute/dev-tools.html.

Could you run rustfmt?

godot-core/src/builtin/quaternion.rs Outdated Show resolved Hide resolved
@Bromeon
Copy link
Member

Bromeon commented Sep 14, 2024

Rebased onto master to resolve the unrelated CI issues (#895).

@theKidOfArcrania
Copy link
Contributor Author

ran rust format and clarified comment

Comment on lines 194 to 204
#[itest]
fn quaternion_mul2() {
use godot::builtin::real;
use std::f32::consts::PI;

let q = Quaternion::from_axis_angle(-Vector3::UP, PI / 2.0 as real);
let rotated = q * Vector3::new(1.0, 4.2, 2.0);
assert_eq_approx!(rotated.x, -2.0);
assert_eq_approx!(rotated.y, 4.2);
assert_eq_approx!(rotated.z, 1.0);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you make one of these 2 tests a non-90° rotation? Maybe run the same in GDScript and check the values. Thanks a lot! 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a non-90° and checked back in godot to make sure numbers are correct.

godot-core/src/builtin/quaternion.rs Outdated Show resolved Hide resolved
@Bromeon Bromeon added this pull request to the merge queue Sep 14, 2024
Merged via the queue into godot-rust:master with commit 6a63f3e Sep 14, 2024
15 checks passed
@Bromeon
Copy link
Member

Bromeon commented Sep 14, 2024

Thanks a lot, nice work on your first pull request! 😊

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: core Core components feature Adds functionality to the library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Quaternion - Vector3 Multiplication operator
3 participants